Repository navigation
Conversation
Signed-off-by: nv-vankit <nv-vankit@users.noreply.github.com>
|
All contributors have signed the DCO ✍️ ✅ |
|
I have read the DCO document and I hereby sign the DCO. |
shailendra-nv
left a comment
There was a problem hiding this comment.
Requesting changes for two safety issues in the stale-alias recovery path.
- The runners can delete and replace an arbitrary persistent user gateway registration named by -GatewayName, including stored auth state, and do not restore the prior registration or active selection. Isolate CLI state per run or preserve and restore it.
- Clearing only OPENSHELL_GATEWAY leaves the higher-precedence OPENSHELL_GATEWAY_ENDPOINT override active, so sandbox create/delete operations can still target the wrong gateway. Clear and restore both overrides or explicitly bind every operational command to the expected endpoint.
Please add regression coverage that preserves a sentinel existing registration and active selection, and that runs with a nonblank OPENSHELL_GATEWAY_ENDPOINT.
| $normalizedExpected = $expectedEndpoint.TrimEnd('/') | ||
| if ($existingEndpoint -ne $normalizedExpected) { | ||
| Info "'$GatewayName' points at '$existingEndpoint' instead of '$normalizedExpected'; replacing the stale registration" | ||
| $removeResult = Invoke-NativeCaptured $cli @("gateway", "remove", $GatewayName) |
There was a problem hiding this comment.
[P1] This removes any user registration matching $GatewayName, including stored OIDC/edge credentials and the active selection, then replaces it with a plaintext test registration without restoring the old state. Run the CLI under a per-run XDG_CONFIG_HOME or preserve and restore the exact prior registration and active gateway. The OCSF runner has the same issue.
| Step "Register CLI -> gateway" | ||
| $env:OPENSHELL_GATEWAY = "" | ||
| $gatewayAdd = Invoke-Cli @("gateway", "add", "http://127.0.0.1:$Port", "--local", "--name", $GatewayName) -AllowFailure | ||
| Remove-Item Env:OPENSHELL_GATEWAY -ErrorAction SilentlyContinue |
There was a problem hiding this comment.
[P1] OPENSHELL_GATEWAY_ENDPOINT has higher routing precedence and remains inherited here. Later create/delete calls can therefore operate on that endpoint; because this runner deletes fixed names such as ocsf1 even after a failed create, it can delete an existing sandbox on the wrong gateway. Clear and restore both overrides or explicitly bind every operational call to the expected endpoint. The E2E runner has the same precedence gap.
Signed-off-by: nv-vankit <nv-vankit@users.noreply.github.com>
Addressed both requested safety issues in
Regression coverage now:
Please re-review the updated head. |
shailendra-nv
left a comment
There was a problem hiding this comment.
Requesting changes on the updated head for a PowerShell 7 isolation regression.
SetEnvironmentVariable(..., $null, "Process") deletes the override under Windows PowerShell 5.1, but under PowerShell 7.6.5 it leaves an empty variable. The CLI treats an empty OPENSHELL_GATEWAY_ENDPOINT as a direct endpoint and fails with invalid URI: empty string; an absent override correctly reports no active gateway.
Please clear variables with Remove-Item "Env:$name" and restore null snapshots by removing the variable. Add a pwsh regression alongside powershell.exe; current harness coverage uses only powershell.exe and a plaintext sentinel.
| [Environment]::SetEnvironmentVariable($entry.Key, $entry.Value, "Process") | ||
| } | ||
| foreach ($name in $cliEnvironmentNames | Where-Object { -not $isolatedPaths.ContainsKey($_) }) { | ||
| [Environment]::SetEnvironmentVariable($name, $null, "Process") |
There was a problem hiding this comment.
[P1] PowerShell 7 converts this null value into an existing empty environment variable. OPENSHELL_GATEWAY_ENDPOINT then resolves as an empty direct endpoint and operational commands fail with invalid URI: empty string. Use Remove-Item on the dynamic Env: path when clearing, and during restoration remove variables whose snapshot was null. Please cover pwsh as well as powershell.exe; the OCSF runner has the same defect at its corresponding clear loop.
There was a problem hiding this comment.
Addressed in c573b22.
Both runners now use Remove-Item "Env:$name" to clear overrides and also remove variables when restoring null snapshots. This avoids empty OPENSHELL_GATEWAY_ENDPOINT values under PowerShell 7.
Regression coverage now runs under both powershell.exe and pwsh.exe, verifying preservation of the sentinel registration, active selection, endpoint override, and absent-variable state.
Please re-review the changes.
Signed-off-by: nv-vankit <nv-vankit@users.noreply.github.com>
Summary
Fix the shipped MXC E2E and OCSF-audit runners when a caller already has a gateway registration using the same alias.
Each runner now uses disposable per-run CLI state, so existing gateway registrations, stored authentication, and the caller’s active selection are never read, replaced, or deleted.
The runners also clear all inherited gateway-selection overrides, including the higher-precedence
OPENSHELL_GATEWAY_ENDPOINT, and restore the caller’s environment during cleanup. Regression tests verify preservation of a sentinel registration and active selection while a nonblank endpoint override is present.Related Issue
Before / after reproduction
Before this change,
bug6870039-e2ewas seeded athttp://127.0.0.1:18000and the E2E runner started its gateway on port 17671. Registration reported that the alias already existed, the CLI continued using port 18000, and the run ended withPASS=0 FAIL=4.After this change, both runners inspect the existing registration. A matching alias is reused, while a stale alias is removed and recreated for the current run.
Validated the full registration matrix:
PASS=4 FAIL=0.PASS=4 FAIL=0.PASS=4 FAIL=0.Changes
OPENSHELL_GATEWAYoverride before registration.gateway addfails.Testing
mise run windows:check:x64mise run windows:test:x64- 5,044 passed, 29 skipped.mise run windows:artifactsPASS=4 FAIL=0.git diff --check.mise run windows:test:mxc-real:x64- 13 passed, 2 unrelated existing failures: a stale IsolationSession request without schemaversion, and an HTTPS proxy probe returning HTTP 403 due to socket-owner/binary identity behavior.Checklist